Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sessions): session permissions storage API #242

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

geekbrother
Copy link
Contributor

This is session permissions off-chain storage API.

The corresponding Notion doc.

@geekbrother geekbrother self-assigned this Jun 13, 2024
Copy link

vercel bot commented Jun 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
walletconnect-specs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 8, 2024 1:10am

@geekbrother geekbrother marked this pull request as ready for review June 13, 2024 15:57
Comment on lines 129 to 135
* `pci` - PCI to revoke.
* `signature` - Signature signed by the key provided during the permission creation.
* `context` - Permissions context object to update.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: put these as comments inline with the type definition for easier reading

@geekbrother geekbrother changed the title feat(sessions): [DRAFT] session permissions storage API feat(sessions): session permissions storage API Jun 26, 2024
@geekbrother geekbrother force-pushed the feat/bcapi_permissions branch 2 times, most recently from f644093 to 982c7ac Compare July 9, 2024 10:18
@geekbrother geekbrother force-pushed the feat/bcapi_permissions branch from 982c7ac to 10733a3 Compare July 9, 2024 10:19
Comment on lines +64 to +77
The POST request body should be in JSON format and following schema:

```typescript
{
permission:
{
"permissionType": string,
"data": string,
"required": boolean,
"onChainValidated": boolean
}
}
```

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request body should have permission struct defined in ERC7715 + extra field onChainValidated if needed but not sure how will Dapp/Client know this( onChainValidated ) information.

Comment on lines 108 to 129

```typescript
{
pci: string,
signature: string,
context: {
signer: {
type: string,
data:{
ids: string[],
}
},
expiry: number,
signerData: {
userOpBuilder: string
},
factory: string,
factoryData: string,
permissionsContext: string
}
}
```

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context field should contain the complete response of wallet_grantPermission request received by the Dapp/client from wallet, because it contains list of granted permissions for requested session. The initial set of permissions stored by the co-signer could be different than final granted permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the context should be the list of granted permissions from ERC7715?

@geekbrother geekbrother force-pushed the feat/bcapi_permissions branch from 977c11e to f89379c Compare October 3, 2024 18:09

### Get permissions list for account

Used to get account list of active sessions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need either a new endpoint for revoked sessions, or including a field in the pci that includes the "status" ; else we cannot displayed previously revoked sessions on the ui

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants